Write the checkpoint that status, resume and handoff already read - #255
Merged
Conversation
Three shipped verbs read one artifact and nothing wrote it. SessionStore.save had no caller anywhere in src/bonfire/ and the pipeline loop had no checkpoint write site, so a bonfire run that dispatched stages and spent money was followed by "No active session". The engine now takes an optional CheckpointSink and hands it the passed stages at every stage-group boundary, which the composition root wires to the same SessionStore the three verbs read. Written per group rather than once on the way out: the value of the record is that it survives a run which does not reach its end. Written before the budget check, not after: that group was paid for whether or not the next line halts. Only gate-passed stages are ever recorded, so a resumed run never skips a stage that failed. Resume does not re-bill -- run(completed=...) skips the named stages and seeds their cost, proved here by counting transport calls rather than re-reading a total the engine computed. The sink takes facts rather than a PipelineResult: a run in progress has no result, and building one at the call site would mean the engine stamping a success value onto a question it has not answered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three entries appended, each sized to a measurement and none padded. src/bonfire/engine/pipeline.py 989 -> 994 (+5) src/bonfire/engine 1903 -> 1981 (+78) tests/integration 1363 -> 1702 (+339) tests/integration measured EXACTLY its ceiling on origin/main, so no pull request could add an integration test at all -- and an integration test through the composition root is the only shape that catches this defect family, because every unit test of the run path injects its own engine factory. Each entry names what the lines buy and the alternatives rejected. The engine numbers are what remain after moving the mechanism out of pipeline.py twice, into session/ and into engine/checkpoint.py. tests/unit is deliberately untouched: a second lane needs that ceiling this round, and these tests belong in tests/integration regardless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two BLE001 suppressions in engine/pipeline.py are pre-existing and already blessed. exemptions.json anchored them by LINE NUMBER (154, 589). Inserting the checkpoint write site above them pushed those lines to 158 and 599, so the anchors stopped matching and cf-exemptions reported two registered exemptions as unregistered. The suppressions did not change. The registry lost its anchor. Re-anchored to the enclosing qualified symbols, which the gate accepts and which do not move when a line is inserted above them: 154 -> PipelineEngine.run 589 -> PipelineEngine._execute_stage Count-neutral: 52 entries in, 52 out, frozen_count untouched. Nothing added, nothing raised, no suppression written. Narrowing these to OSError was considered and rejected on evidence: it turns 10 tests red, including the whole outer-exception parity suite and the unregistered-gate refusal, because both sites exist precisely to convert ANY failure into a typed result rather than crash a run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect, verified in this lane before building
SessionStore.savehad no caller anywhere insrc/bonfire/, and the pipeline loop had no checkpoint write site.bonfire status,bonfire resumeandbonfire handoffall read that artifact. Three shipped verbs, no producer.Verified with my own greps rather than on the inherited report:
grep -rn "SessionStore" src/— 3 readers (status,resume,handoff), 0 writers.grep -rn "\.save(" src/— exactly one hit,store.py:85, insideSessionStore.saveitself. Nothing calls it.grep -n "checkpoint" src/bonfire/engine/pipeline.py— 3 hits, all docstring/comment.composition.py— zero.Then reproduced empirically: a real 2-stage run through
build_default_enginewith the SDK transport faked,BONFIRE_CHECKPOINT_DIRpointed at an empty dir.Everything in the brief reproduced. Two claims I want to correct in passing, both mine, neither a contradiction of the brief:
src/bonfire/engine/__init__.pyanddocs/architecture.mdboth documented this as intended design ("The engine does not write checkpoints between stages; callers persist aPipelineResult"). So it was a documented extension surface with no in-tree caller, not an oversight nobody had noticed. I corrected the package docstring;docs/architecture.mdis another lane's this round and is now stale (see Owed).Where the checkpoint is now written
PipelineEnginetakes an optionalcheckpoint_sink: CheckpointSink | None. At every stage-group boundary it hands the passed stages to that sink.build_default_enginewires it to the sameSessionStorethe three verbs read, so one directory is resolved by one rule (BONFIRE_CHECKPOINT_DIR, then~/.bonfire/checkpoints) rather than two resolutions agreeing by coincidence.Per stage group, not once at the end — checked against the three things the brief asked about:
CheckpointManager.savewrites a.tmpandos.replaces it, throughsafe_write_textwith its symlink refusals. This matters more now, not less: the write now happens repeatedly while the run is alive, so an interruption mid-write is an ordinary outcome rather than a freak one. It leaves either the previous checkpoint or the new one, never a lying record.{session_id}.json, so two runs never collide on a file. Butlatest()picks max-timestamp across the directory, so a second concurrent run does shadow the first forstatus/resume. That is pre-existing behaviour oflatest(), not introduced here, and I did not change it.Written before the budget check, not after. That group was dispatched and paid for whether or not the next line halts the run; a halt that discarded the record would bill it again on the next attempt.
Only gate-passed stages are ever recorded.
stages_doneat the call site holds only stages that cleared their gates. Recording a failed stage would be worse than recording nothing, because resume skips whatever the checkpoint names — a failed stage written as done is a stage that silently never runs again.What the three verbs do with it, run as real separate processes (not
CliRunner):A 2-stage
debugrun halted by the budget check afterscout, then three realbonfireprocesses reading only the file it left:Every one of those three printed an absence-of-state line before this PR.
Ruling on re-billing
A resume does not re-dispatch and does not re-bill.
run(plan, completed=...)skips the named stages in the DAG and seedstotal_costfrom their envelopes, so prior spend still counts againstplan.budget_usdand cannot be spent twice.Measured at the transport, not inferred from a total the engine computed: the first leg halts after
scout(2 transport calls), the checkpoint is fed back, the second leg makes 2 transport calls for the two remaining stages and ends at$0.33= 3 x$0.11.scoutis never re-dispatched, and its output is carried forward intowarrior's prompt, which is what makes skipping it legitimate rather than merely cheaper.The boundary, stated plainly:
bonfire resumecomputes and reports the re-entry; it does not itself dispatch. That is its existing documented contract and I did not change it. What changed is that it now has real data to act on — before this PR it could only ever print "No session to resume." Making the verb actually re-enter the pipeline is a separate ruling for Anta: it turns a currently free verb into a billing verb, and the dispatch driver (cli/commands/run.py) is outside this lane's partition.Control rods — both halves
Rod A — the producer. Removed the write site from
_run_inner.Verbatim reasons:
Rod B — the composition root. Restored A, then removed
checkpoint_sink=SessionStore()frombuild_default_engine. The same four behavioural tests plus the wiring assertion go red. Verbatim, and this is the one that matters:A real
bonfire statusprocess printing the pre-fix message while the run it should describe sits in the same test. This is the half a hand-assembled engine cannot catch — every one of these tests goes throughbuild_default_engine, so unwiring the root is visible. Under Rod B the two tests that must not depend on the write (test_an_engine_with_no_sink_writes_nothing, the Protocol conformance test) stayed green, so the rod discriminates rather than just breaking everything.Both restores via
git checkout— byte-identical by construction,git statusclean, 8/8 green after each.Budget — measured, and three raises taken
Measured
cf-file-budget checkas an early action, before authoring. Findings at that point:src/bonfire/engine1 line of headroom,tests/integration0,tests/unit1.Restructured twice to shrink the ask before raising anything:
PipelineResultconstruction + the durability reasoning moved toSessionStore.save_progress—session/is unbudgeted and is the layer that owns what a stored record means.sink is Noneguard + error handling moved towrite_progressinengine/checkpoint.py— the module that already owns the atomic write and the symlink refusals.That took
pipeline.pyfrom +52 down to +5, and the engine package from +78-with-a-fat-pipeline to +78 spread properly.src/bonfire/engine/pipeline.pysrc/bonfire/enginetests/integrationCommitted separately (
ab63a61), appended at the end ofpackage_raises, three number changes + three entries, nothing reformatted. Each carries what the lines buy and the alternatives rejected in my own words. Headline rejections:_run_inner. Costs zero new lines inpipeline.py— but adds two branches to a function frozen in the complexity snapshot at 23.bonfire/protocols.py. Unbudgeted, would have cost zero — but CLAUDE.md's release gate names "the four@runtime_checkableextension protocols" as a v0.1 trust-triangle item, and quietly making it five edits a documented count from a lane that does not own that doc.eventspackage. Not a budget trade — it does not work.StageCompletedcarriesstage_name,agent_name,duration_seconds,cost_usdand no Envelope, so a consumer cannot populateCheckpointData.completedat all.tests/unit. Deliberately untouched: a second live lane needs that ceiling this round, and hand-building the engine there is the measurement error this defect family is made of.exemptions.jsonuntouched.tests/unituntouched.Gates
_run_innerand_execute_stagestay at their snapshot complexities — the change adds a call, not a branch.What I did NOT prove
claude_agent_sdk.querythroughout.bonfiresubprocesses reading a file the pytest process wrote, plus the pre-existing atomicity test ontmp+os.replace. I did not SIGKILL a run between thesafe_write_textand theos.replace.latest()shadowing is unproven either way and unchanged by this PR.resumedoes not dispatch. Proved that its computed inputs, fed to the engine, do not re-bill. Did not prove a CLI-driven resume end to end, because the verb does not do that.status/resumederive the stage total and the remaining list from the registry keyed byplan_name, not from the checkpoint. Found while writing these tests: a plan name whose registered shape differs from what actually ran is graded against the registered shape. Harmless today (names always come from the registry) but it means a workflow whose stages change between run and resume will compute its remainder against the new plan. Not fixed — out of this lane's scope, and it is the registry's contract to settle.Owed
docs/architecture.mdlines ~168 and ~219-241 now describe the old behaviour ("PipelineEngine.run()does not write checkpoints", "SessionStore.saveexists and has no caller").docs/is another lane's this round, so I left it. It needs the correction.CHANGELOG.mduntouched by instruction. The line this lane would have written:bonfire runnow writes a checkpoint at every stage boundary, sobonfire status,bonfire resumeandbonfire handoffreport a real run instead of an empty store.Follow-up:
cf-exemptionswent red in CI, and why the fix is a re-anchorCI's
gate / gatefailed on:Neither suppression is mine and neither is new. Both exist on
7bddd71at lines 154 and 589, and both are already registered inexemptions.json(entries 15 and 16, blessed under the Elegance Law).exemptions.jsonanchors by line number. My insertions pushed 154 -> 158 and 589 -> 599, so the anchors stopped matching.The suppressions did not change. The registry lost its anchor. The gate's message ("a self-issued suppression is not an exemption") cannot tell those two cases apart.
Proof:
cf-exemptionsis OK on bare7bddd71and FAILs on this branch on exactly those two rows.Narrowing them to
OSErrorwas tried and rejected on evidence. It makescf-exemptionspass and turns 10 tests red:pipeline.py:158is the barrier behind the documented contract "PipelineEngine.run()NEVER raises";:599wrapshandler.handle()whereStageHandleris an open-set Protocol. Narrowing them would also re-open the unregistered-gate defect PR #251 landed yesterday. TheOSErrorprecedent from #249 is right for new defensive handling around a disk write — and my own checkpoint write already usesexcept (OSError, ValueError)inwrite_progress. It is wrong for these two.Fix: re-anchored by qualified symbol, which the gate accepts (
symbol_or_line == str(line) or == suppression.symbol) and which does not drift:Symbols computed with the gate's own
_symbol_spans/_enclosing_symbol, not guessed. Both unique in the file, one suppression each, so no symbol swallows a second site.Count-neutral: 52 entries in, 52 out,
frozen_countuntouched. Nothing added, nothing raised. The diff is two lines.Two rods on the re-anchor:
# noqa: BLE001inPipelineEngine._emit:cf-exemptions: FAIL (1 violation(s)). Not defanged.cf-exemptions: OK. A corrected number would have broken again on the next insertion.The real finding: line-anchored exemptions silently un-register
A registry that anchors suppressions by line number un-registers them whenever anything is inserted above. Two lanes hit this in the same hour on different files (this one on
engine/pipeline.py, a sibling ononboard/flow.pyandonboard/orchestrator.py). That is a defect in the gate's design, not in either lane's code, and it will keep firing until the remaining 50 numeric anchors become symbols. This PR converts 2 of them.Why local did not equal CI
Not tool availability —
cf-exemptionswas installed and working locally the whole time. I hand-picked four gate binaries (cf-file-budget,ruff,lint-imports,complexipy) instead of running the aggregatecf-gatethat CI runs, so the fifth gate was never invoked.Running
cf-gatelocally is itself not a clean signal: on bare7bddd71it reportsFAIL - 4 of 11(cf-import-contract,mypy,complexipy,pytest) because the kit venv has no installedbonfireto resolve. The workable local==CI rule in this setup is: runcf-gateand diff its verdict against bareorigin/main, and treat only the delta as yours. Against that baseline this branch's only delta wascf-exemptions, now green.